-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GitHub Actions: Prevent ctest invocation error on macOS #2479
Conversation
@nico @jhasse @digit-google I know you all must be very busy, sorry to be impatient on this one, but this is just 1 line that fixes the CI test. Can we do it? My other PR's CI test states:
|
a similar fix is part of #2041 |
Correct--for most of CMake's history, CTest would ignore invalid flags, which was quite confusing. As in this case, where "-vv" did nothing, but now to help avoid confusion, CTest errors on invalid command line flags. |
I would prefer a version which avoids ctest all together. Not sure what the advantages of it are besides issue likes this. There were other issues with ctest in the past and my general opinion of it is, that it should be avoided at all costs and nuked from existence. |
aad76e5
to
c0f1521
Compare
@jhasse ok. no more |
I'm not going to bother changing the title of the PR, if that's ok... |
1963bf8
to
a2df75c
Compare
@jhasse I dug deeper and found out that
|
It seems that new versions of CTest now require case-sensitive arguments, resulting in an "unknown argument" error for "-vv", while documentation shows the argument for the full verbosity option as "-VV". In CMakeLists.txt, there is currently only 1 test registered to CMake with add_test(), and that is to run the ninja_test binary. Since using CTest is not reducing the number of commands used for testing, fix the testing invocation by replacing CTest with a direct run of the test. Signed-off-by: Michael Pratt <[email protected]>
a2df75c
to
6e77cdf
Compare
nevermind after reading more documentation I see it is considered a multi-config generator even though many people do not use it that way. so now, just correcting the path |
Thank you for this, I am glad we can get rid of CTest too. @jhasse, can we submit this asap to unblock all other PRs that fail their checks because of this issue? |
Thanks! |
ping @jhasse @nico
It seems that new versions of CTest now require case-sensitive arguments.
To prevent errors related to case-sensitivity of arguments,
use the long option form for verbosity, and fallback to printing out the help message and running normally.get rid of CTest.While at it, print the version in all cases for reference.